Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Testing] Externalize (find_package() or fetch) googletest/gtest #4471

Merged
merged 7 commits into from
Feb 7, 2024
Merged

[Testing] Externalize (find_package() or fetch) googletest/gtest #4471

merged 7 commits into from
Feb 7, 2024
+1,700 −36,805

Conversation

fredroy
Copy link
Contributor

@fredroy fredroy commented Jan 31, 2024 •
edited
Loading

Originally, the goal was not really to go for

But more to upgrade the gtest version, as the one in SOFA is really old (1.7.0 , Sep 19, 2013 ❗ ) and was throwing warnings for newer cmake.
So instead of simply update the source in extlibs, I took the liberty to go into the find/fetch mechanism.

A bit difficult as the CMakefile in the embedded version was heavily customized, so I am not 100% sure the install/package process is working well. But it should 😅

Last note: it is adding a target gtest_main which is always activated in the gtest cmake file. It is not used by SOFA as we have our own main for gtest.

[ci-depends-on https://github.com/sofa-framework/PluginExample/pull/5]

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@fredroy fredroy added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request pr: test PR adding test(s) in SOFA labels Jan 31, 2024
@fredroy
Copy link
Contributor Author

fredroy commented Jan 31, 2024

[ci-build][with-all-tests]

@fredroy
Copy link
Contributor Author

fredroy commented Jan 31, 2024

[ci-build][with-all-tests][force-full-build]

@sofabot
Copy link
Collaborator

sofabot commented Jan 31, 2024

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

@olivier-roussel olivier-roussel mentioned this pull request Jan 31, 2024
Closed
@hugtalbot hugtalbot added the pr: backport todo This PR will be backported into the release preceeding its milestone. label Jan 31, 2024
@sofabot
Copy link
Collaborator

sofabot commented Jan 31, 2024

[ci-depends-on] detected during build #5.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

olivier-roussel
olivier-roussel suggested changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@olivier-roussel olivier-roussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the following adjustments, this compiles both using fetched gtest or using external gtest package from conda (tested on linux-x64 with gtest 1.14.0 from conda-forge).

Sofa/framework/Testing/CMakeLists.txt Outdated Show resolved Hide resolved
Sofa/framework/Testing/CMakeLists.txt Outdated Show resolved Hide resolved
Sofa/framework/Testing/CMakeLists.txt Outdated Show resolved Hide resolved
Sofa/framework/Testing/CMakeLists.txt Outdated Show resolved Hide resolved
@fredroy
Copy link
Contributor Author

fredroy commented Feb 2, 2024

@olivier-roussel thanks for the conda test and the feedbacks 👍
You should have made suggestions and/or make a PR on my branch, as I cannot credit you your patches 😵

@sofabot
Copy link
Collaborator

sofabot commented Feb 2, 2024

[ci-depends-on] detected during build #6.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@sofabot
Copy link
Collaborator

sofabot commented Feb 2, 2024

[ci-depends-on] detected during build #7.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@sofabot
Copy link
Collaborator

sofabot commented Feb 2, 2024

[ci-depends-on] detected during build #8.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@fredroy
Copy link
Contributor Author

fredroy commented Feb 2, 2024

[ci-build][with-all-tests][force-full-build]

@sofabot
Copy link
Collaborator

sofabot commented Feb 2, 2024

[ci-depends-on] detected during build #9.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@sofabot
Copy link
Collaborator

sofabot commented Feb 2, 2024

[ci-depends-on] detected during build #10.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@fredroy
Copy link
Contributor Author

fredroy commented Feb 2, 2024 •
edited
Loading

@olivier-roussel
The findGTest https://cmake.org/cmake/help/v3.19/module/FindGTest.html of cmake only works for CMake >= 3.20 😭
(tested with cmake 3.16 and 3.19 ❌ whereas cmake 3.20 and 3.28 ✅)

@fredroy fredroy force-pushed the fetch_gtest branch 3 times, most recently from cbfc102 to eacd343 Compare February 2, 2024 07:34
@sofa-framework sofa-framework deleted a comment from sofabot Feb 2, 2024
@sofa-framework sofa-framework deleted a comment from sofabot Feb 2, 2024
@sofa-framework sofa-framework deleted a comment from sofabot Feb 2, 2024
@olivier-roussel
Copy link
Contributor

@olivier-roussel thanks for the conda test and the feedbacks 👍 You should have made suggestions and/or make a PR on my branch, as I cannot credit you your patches 😵

Indeed, sorry about that

@olivier-roussel
Copy link
Contributor

@olivier-roussel The findGTest https://cmake.org/cmake/help/v3.19/module/FindGTest.html of cmake only works for CMake >= 3.20 😭 (tested with cmake 3.16 and 3.19 ❌ whereas cmake 3.20 and 3.28 ✅)

Indeed again 😢
Another option but maybe a bit conda-specific would be to switch to config mode find_package(GTest CONFIG QUIET), as the conda package comes with the GTest cmake config files. Tested with conda + cmake 3.19.

@olivier-roussel
Copy link
Contributor

Ok so maybe I misunderstood your last comment, but the GTest find module you added in your last commit works fine on my side with cmake >= 3.12 (which is our min required version). So I guess you meant that the embedded GTest find module in recent cmake version (>=3.20) was required to have the correct targets defined, which is not the case in earlier <3.20 version of cmake (tested in 3.12), but this recent GTest find module was not building if used with cmake <3.20. In the end, your last commit seems to fix everything, so looks like a more generic option that relying on cmake config files as suggested in my last comment.

@fredroy
Copy link
Contributor Author

fredroy commented Feb 2, 2024 •
edited
Loading

Ok so maybe I misunderstood your last comment, but the GTest find module you added in your last commit works fine on my side with cmake >= 3.12 (which is our min required version). So I guess you meant that the embedded GTest find module in recent cmake version (>=3.20) was required to have the correct targets defined, which is not the case in earlier <3.20 version of cmake (tested in 3.12), but this recent GTest find module was not building if used with cmake <3.20. In the end, your last commit seems to fix everything, so looks like a more generic option that relying on cmake config files as suggested in my last comment.

I just added the cmake module from v3.20 for GTest directly in our cmake folder modules; not very clean but it does the job apparently 👍

@sofabot
Copy link
Collaborator

sofabot commented Feb 6, 2024

[ci-depends-on] detected during build #14.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@bakpaul bakpaul added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Feb 7, 2024
@sofabot
Copy link
Collaborator

sofabot commented Feb 7, 2024

[ci-depends-on] detected during build #15.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@alxbilger alxbilger merged commit 4599681 into sofa-framework:master Feb 7, 2024
10 of 11 checks passed
@olivier-roussel olivier-roussel mentioned this pull request Feb 8, 2024
Merged
bakpaul pushed a commit that referenced this pull request Feb 19, 2024
@fredroy @bakpaul
[Testing] Externalize (find_package() or fetch) googletest/gtest (#4471)
7602c8e 
* find_package gtest or fetch gtest ; and use it for testing.Remove usage of the old one

* fix compilation for newer gtest

* remove source of the embedded gtest

* update cmake message for SOFA_ALLOW_FETCH_DEPENDENCIES

* fix define

* apply suggestion by using GTest ; and remove CONFIG keyword

* use cmake 3.20 version for gtest modules
@bakpaul bakpaul added this to the v23.12 milestone Feb 20, 2024
@bakpaul bakpaul added pr: backport done This PR has been backported into the release before its milestone. and removed pr: backport todo This PR will be backported into the release preceeding its milestone. labels Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivier-roussel olivier-roussel olivier-roussel approved these changes

Assignees
No one assigned
Labels
enhancement About a possible enhancement pr: backport done This PR has been backported into the release before its milestone. pr: status ready Approved a pull-request, ready to be squashed pr: test PR adding test(s) in SOFA
Projects
None yet
Milestone
v23.12
Development

Successfully merging this pull request may close these issues.
6 participants
@fredroy @sofabot @olivier-roussel @alxbilger @hugtalbot @bakpaul